refactor: use cwltool loader for standardized cwl doc handling - #130
refactor: use cwltool loader for standardized cwl doc handling#130amaltaro wants to merge 4 commits into
Conversation
aldbr
left a comment
aldbr
left a comment
There was a problem hiding this comment.
Thanks for your contribution!
I just have some minor comments 🙂
| # Show basic info | ||
| cwl_version = cwl.get("cwlVersion", "Unknown") | ||
| doc = cwl.get("doc", cwl.get("label", "")) | ||
| cwl_version = getattr(cwl, "cwlVersion", "Unknown") |
There was a problem hiding this comment.
Just wondering if getattr is not misleading in this context, as I think the cwl instance you get will always have cwlVersion, doc, inputs, .... To double check, but I think they just exist with default value like None or [] if nothing is provided.
This would likely work:
| cwl_version = getattr(cwl, "cwlVersion", "Unknown") | |
| cwl_version = cwl.cwlVersion # default value is likely 1.2, to double check | |
| cwl_version = cwl.cwlVersion or "Unknown" # if there is no default value |
There was a problem hiding this comment.
Thank you for the review, @aldbr .
Yes, directly accessing the object attributes should work as well. However, the getattr adds some protection in case the attribute changes for any reason (bug, non-backward compatible changes, etc).
What is the preferred behavior for this module:
a) have a clear exception being raised in case one of the expected attributes is not to be found?
b) or to gracefully deal with this with getattr and default values?
Given that this is already in a try/except block, I think option a) (which is your suggestion) is better. But better to get a confirmation please.
There was a problem hiding this comment.
We discussed and we think the first option is the way to go
|
|
||
| console.print() | ||
|
|
||
| # Show final outputs (handle both dict and list formats) |
There was a problem hiding this comment.
I guess this comment is not true anymore?
| # Show final outputs (handle both dict and list formats) |
|
@ryuwd @aldbr I applied the changes requested during review. However, one of the CI tests fails with an error unrelated to this PR: It looks like another PR that has recently been merged is not failing these unit tests (commit message length). Please let me know how to proceed here and/or how to retrigger CI tests. |
3 participants
Fixes #108
With this pull request, we adopt the cwltool loader functionality to normalize and handle CWL documents, instead of manually juggling with fields and different data types.
Tested with
pixi run dirac-cwl-run test/workflows/crypto/description.cwl --print-workflow, where exactly the same workflow visualization is retained.Also added some unit tests that can be triggered with
pixi run pytest test/test_job_executor.py